fix(snapchat): proxy bundled config loads - #823
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds typed SDK patches and AST passes that pin Snapchat host detection and rewrite script-loader config URLs through the Nuxt proxy. Snapchat registry bundle and proxy configurations now use these patches. Unit, integration, and e2e tests cover rewriting, forced bundling, cache paths, same-origin requests, and fixture behavior. Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/unit/bundle-sdk-patches.test.ts (1)
151-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
snapchatPixelconfig duplicates registry.ts; risks drift.This inline
proxyConfigs.snapchatPixelmirrors the actual registry entry (packages/script/src/registry.ts) but isn't sourced from it, unlike the analogous suite intest/unit/rewrite-ast.test.tswhich callsgetProxyConfigs(). If the registry'ssdkPatches/domainsforsnapchatPixelchange, this test will keep passing against a stale snapshot instead of failing.♻️ Suggested refactor: derive from registry
- proxyConfigs: { - snapchatPixel: { - domains: ['sc-static.net', 'tr.snapchat.com', 'pixel.tapad.com'], - sdkPatches: [ - { type: 'replace-new-url-host', host: 'sc-static.net' }, - { type: 'replace-script-loader-url', fromDomain: 'tr.snapchat.com', pathPrefix: '/config' }, - ], - } as any, - }, + proxyConfigs: { + snapchatPixel: (await buildProxyConfigsFromRegistry(await registry())).snapchatPixel as any, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/bundle-sdk-patches.test.ts` around lines 151 - 182, The Snapchat proxy test is hardcoding a stale `proxyConfigs.snapchatPixel` snapshot instead of using the shared registry data. Update the `applies snapchat config host patches to bundled proxy scripts` test to derive `proxyConfigs` from the registry source used elsewhere (for example via the same helper pattern as `rewrite-ast.test.ts` and `getProxyConfigs()`), so `sdkPatches` and `domains` stay aligned with `registry.ts` and future registry changes are caught.test/unit/transform.test.ts (1)
645-687: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
hashmock or assert the cache key.vi.mocked(hash).mockImplementationOnce(() => 'scevent.min')only affects the intermediate source filename/cache key; the emitted asset URL still comes from the fetched content hash. If this test isn’t checking cache naming, the mock is unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/transform.test.ts` around lines 645 - 687, The `transform` test for `useScriptSnapchatPixel` is over-mocking `hash` in a way that only affects the intermediate cache key, not the emitted asset URL. Remove the `vi.mocked(hash).mockImplementationOnce(...)` stub unless the test explicitly asserts cache-key behavior, or update the expectations to verify the cache key produced by the `bundle` path and its interaction with `mockBundleStorage`. Keep the assertions focused on the actual `transform` output and the fetch bypass behavior.test/fixtures/first-party/nuxt.config.ts (1)
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGrammatically broken comment.
"Pages that need the real SDK load provide an explicit trigger" doesn't parse. Likely meant "Pages that need the real SDK should provide an explicit trigger...".
✏️ Suggested wording fix
// trigger: 'manual' prevents the auto-generated plugin from loading all 18 -// scripts globally on every page. Pages that need the real SDK load provide an -// explicit trigger in their composable call, keeping cross-provider noise low. +// scripts globally on every page. Pages that need the real SDK should provide +// an explicit trigger in their composable call, keeping cross-provider noise low.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/fixtures/first-party/nuxt.config.ts` around lines 4 - 5, The inline comment in the Nuxt config is grammatically broken and should be rewritten for clarity. Update the descriptive text near the existing explanatory comment so that the sentence about pages needing the real SDK reads naturally, using the same surrounding context in the configuration block. Keep the meaning intact while fixing the wording in the comment text itself.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/fixtures/first-party/nuxt.config.ts`:
- Around line 4-5: The inline comment in the Nuxt config is grammatically broken
and should be rewritten for clarity. Update the descriptive text near the
existing explanatory comment so that the sentence about pages needing the real
SDK reads naturally, using the same surrounding context in the configuration
block. Keep the meaning intact while fixing the wording in the comment text
itself.
In `@test/unit/bundle-sdk-patches.test.ts`:
- Around line 151-182: The Snapchat proxy test is hardcoding a stale
`proxyConfigs.snapchatPixel` snapshot instead of using the shared registry data.
Update the `applies snapchat config host patches to bundled proxy scripts` test
to derive `proxyConfigs` from the registry source used elsewhere (for example
via the same helper pattern as `rewrite-ast.test.ts` and `getProxyConfigs()`),
so `sdkPatches` and `domains` stay aligned with `registry.ts` and future
registry changes are caught.
In `@test/unit/transform.test.ts`:
- Around line 645-687: The `transform` test for `useScriptSnapchatPixel` is
over-mocking `hash` in a way that only affects the intermediate cache key, not
the emitted asset URL. Remove the `vi.mocked(hash).mockImplementationOnce(...)`
stub unless the test explicitly asserts cache-key behavior, or update the
expectations to verify the cache key produced by the `bundle` path and its
interaction with `mockBundleStorage`. Keep the assertions focused on the actual
`transform` output and the fetch bypass behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd56888e-76f0-4664-a05b-f77b2325e04d
📒 Files selected for processing (9)
packages/script/src/plugins/rewrite-ast.tspackages/script/src/registry.tspackages/script/src/runtime/types.tstest/e2e-dev/first-party.test.tstest/fixtures/first-party/nuxt.config.tstest/fixtures/first-party/pages/snap.vuetest/unit/bundle-sdk-patches.test.tstest/unit/rewrite-ast.test.tstest/unit/transform.test.ts
…ed subtrees Rewriting a loader argument then descending into it could produce conflicting MagicString edits when the argument contained a literal the domain-rewrite pass also matched. Skip the replaced subtree instead. Match each replace-script-loader-url patch against its own pathPrefix so multiple patches can't rewrite a path to the wrong proxied domain.
b756a68 to
26a2713
Compare
🔗 Linked issue
Resolves #822
❓ Type of change
📚 Description
Bundled
scevent.min.jswas deriving its config host from the local asset URL, then loading/config/...from the app origin. This adds scoped SDK patches fornew URL(...).hostand script-loader URL arguments so Snapchat config loads go through thetr.snapchat.comproxy without depending on current minified names.The Snapchat first-party e2e now forces a fresh upstream bundle and fails if
/config/...is requested from the app origin. The bundled SDK regression test also derives Snapchat proxy settings from the registry so domains and patches cannot drift silently.